[FIX] Redirect logged-in users from / to /home - #470
Conversation
If a session cookie is valid, the landing page now sends users to /home instead of asking them to sign in again. Kubernetes production now sets ENV=production so Secure cookies match Docker. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thank you for opening this PR! Before a maintainer takes a look, it would be really helpful if you could walk through your changes using GitHub's review tools. Please take a moment to:
More information on how to conduct a self review: This helps make the review process smoother and gives us a clearer understanding of your thought process. Once you've added your self-review, we'll continue from our side. Thank you! |
NaitikVerma6776
left a comment
There was a problem hiding this comment.
Self-review of the leftover #417 pieces from current main.
Scope: landing / → /home when a session exists, K8s ENV=production (the #420 leftover), and tests for cookie Secure/MaxAge. Auth middleware is intentionally unchanged — that rewrite in #462 was questioned in review and is already present on main in the form that landed.
Empty tasks in the UI (CLI sync still worked) is out of scope and not claimed as fixed.
A short verification video (logged in, visit / → /home; refresh later still logged in) will be attached here or on #417.
| credentials: 'include', | ||
| }); | ||
| if (response.ok) { | ||
| navigate('/home'); |
There was a problem hiding this comment.
This is the user-visible fix for bookmarking / while already logged in.
Same check HomePage already uses (GET api/user + credentials: 'include' so the session cookie is sent). If the session is valid we navigate('/home'). If it is 401/network error we stay on the landing page so first-time / logged-out visitors can still sign in.
I did not add a loading gate — a brief flash of the landing page is possible before redirect. Happy to add a spinner if maintainers want that.
| expect(fetch).toHaveBeenCalledWith('http://mocked-backend-url/api/user', { | ||
| method: 'GET', | ||
| credentials: 'include', | ||
| }); |
There was a problem hiding this comment.
Default fetch mock is ok: false so existing render/snapshot tests still show the landing page.
This case asserts both the redirect and that the request includes credentials (the cookie is what makes production / work after login). The next two tests cover logged-out (ok: false → no navigate) and a thrown fetch (stay on landing, console.error).
| Path: "/", | ||
| MaxAge: 86400 * 7, // 7 days | ||
| HttpOnly: true, // Prevent JavaScript access | ||
| Secure: os.Getenv("ENV") == "production", // HTTPS only in production |
There was a problem hiding this comment.
No behavior change vs what is already on main: 7-day MaxAge, Secure only when ENV=production, SameSite=Lax.
Pulled into a helper only so we can unit-test those flags (review on #462 asked for session tests). I did not take #462's 30-day lifetime or the X-Forwarded-Proto Secure rewrite — that was the part that got questioned, and main already has working session + AuthMiddleware.
| assert.Equal(t, "/", opts.Path) | ||
| assert.Equal(t, 86400*7, opts.MaxAge) | ||
| assert.True(t, opts.HttpOnly) | ||
| assert.True(t, opts.Secure) |
There was a problem hiding this comment.
Covers the two cookie properties that matter for #417 on HTTPS: Secure=true in production (browsers drop non-Secure cookies on https://taskwarrior-server.ccextractor.org), and MaxAge=7d so a refresh the next day should still be logged in.
The other two tests lock Secure=false when ENV is development or unset, so local HTTP Docker still works.
| CLIENT_ID: "YOUR_GOOGLE_CLOUD_AUTH_CLIENT_ID" # Replace this in order to access the frontend | ||
| CLIENT_SEC: "YOUR_GOOGLE_CLOUD_AUTH_CLIENT_SECRET" # Replace this in order to access the frontend | ||
| CONTAINER_ORIGIN: http://syncserver:8080/ | ||
| ENV: "production" # Required for secure HTTPS-only cookies |
There was a problem hiding this comment.
Same leftover as #420. Docker already had ENV=production (production/example.backend.env). Without this key, K8s cookies stay Secure=false and browsers reject them on HTTPS — users then look "logged out" every visit.
Must stay paired with the ENV envFrom wiring in backend-deployment.yaml; a configmap value that is never mounted is a no-op.
| valueFrom: | ||
| configMapKeyRef: | ||
| key: ENV | ||
| name: backend-env |
There was a problem hiding this comment.
Wires the configmap ENV into the backend container the same way the other secrets/config keys are wired. This is the half of #420 that is easy to miss: adding the key to the configmap alone does not set os.Getenv("ENV") in the process.
| Secure: os.Getenv("ENV") == "production", // HTTPS only in production | ||
| SameSite: http.SameSiteLaxMode, // CSRF protection (Lax allows OAuth redirects) | ||
| } | ||
| store.Options = sessionCookieOptions() |
There was a problem hiding this comment.
Call-site only. Options values are unchanged. Auth middleware / session save path left as-is on purpose — no third copy of the #462 rewrite.
Description
When a user is already logged in, visiting
/now checksapi/userwithcredentials: includeand redirects to/home. Kubernetes production now setsENV=productionso session cookies get the Secure flag (same leftover as #420). Session cookie options stay 7-day MaxAge and Secure only whenENV=production; those options are covered by unit tests. Auth middleware is unchanged.This is a small PR from current main. It does not rewrite session helpers from #462.
Not in this PR: empty tasks in the UI while CLI sync works. That report is out of scope unless reproduced separately.
Leftover from earlier PRs
ENV=productiononly. Still missing on main. This PR includes that wiring (configmap + deployment). Docker already hadENV=production.ENV=production, AuthMiddleware) is already on main. LandingPage on main still had no logged-in redirect. Review asked for tests and questioned the auth middleware. This PR takes only the landing redirect + tests, not a second copy of the auth rewrite.Checklist
npx prettier --writeon the changed frontend filesgofmt -w .(Go was not available in a usable local install; CI will format/test)npm test -- --testPathPattern=LandingPageAdditional Notes
A short verification video will be attached on this PR or on #417 (logged in, visit
/→/home; refresh later still logged in).Video.Project.mp4